Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check we don't overflow when casting down integers during parsing #4353

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

ArnaudBienner
Copy link

Hi,

I've faced the following issue when using this library: I stored some numbers in small integers (because I expect to use values in a small interval in practice) but the static_cast made during parsing lead to invalid values because of the overflow.

I believe it would be nice to have this runtime check to notice possible overflows at runtime.

const auto val = *j.template get_ptr<ArithmeticTypeSource*>();
const auto min = std::numeric_limits<ArithmeticTypeTarget>::min();
const auto max = std::numeric_limits<ArithmeticTypeTarget>::max();
if (val < min && val > max)
Copy link
Contributor

@gregmarr gregmarr Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if will never be true, unless you get some weird results from val and min/max having different types. You want || not &&. You probably also want min and max to have the same type as val.

You would also only need to do this test if ArithmeticTypeTarget and ArithmeticTypeSource are different types, and then only if Target is smaller than Source, or if they have different signed-ness.

It might also be better to do this as a pre-test that just throws on out of range, and leave the current lines alone for the actual assignments.

@nlohmann
Copy link
Owner

I am a bit concerned that adding overflow exceptions (though correct) are changing the behavior of the library in a breaking (and maybe also surprising) way.

Copy link

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @ArnaudBienner
Please read and follow the Contribution Guidelines.

@coveralls
Copy link

Coverage Status

coverage: 100.0%. remained the same
when pulling da5800c on ArnaudBienner:develop
into 8c391e0 on nlohmann:develop.

* Make the check optional if the target type is greater or equal to source type, or if source is floating point number
* Ignore infinity values, as they are already handled
@ArnaudBienner
Copy link
Author

Thank you all for your feedback :)

Turns out the check was (by chance) working in the sample case I wrote for unit testing :( I should have been more careful and more exhaustive in my testing, sorry about that. I've added a few more unit tests.

In addition to fixing the check, I also change the code to:

  • Not perform the check if the source type is smaller than the destination type: as discussed, this is not needed in this case. I didn't managed to use enable_if to have two different version of that function. In my PR I'm using if constexpr to achieve a similar effect, but this is a C++17 feature, even if I know this project is C++11: this was just to make a point. I can either drop it completely, or if you have a way to define macros to support multiple C++/compilers versions and features, we can do this as well.
  • Always perform the check in case the source number is floating points: in this case the sizeof check is not relevant
  • Ignore the check when dealing with infinity numbers: I realized when running unit tests this was already handled specifically in the code elsewhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants